feat(library): Add rating sync with file tags (FMPS_RATING standard) - #15952

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

Thanks for the review! All four items addressed in the latest push:

1. Smart pointer: Removed RatingSyncWorker entirely, so the raw pointer is gone. The existing SoundSourceProxy::updateTrackFromSource() pipeline already handles rating import via syncParams.importRatingFromFile, making the custom worker unnecessary.

2. snake_case config keys: Renamed to export_rating_to_file_tags and import_rating_from_file_tags.

3. Why custom worker?: See master sync first merge #1 — we don't need one. Removed it along with all references in baseplaylistfeature and cratefeature.

4. Tests: Added ratingexportimport_test.cpp with:

   • Parameterized round-trip test across all 8 supported formats (MP3, FLAC, OGG, Opus, M4A, WAV, AIFF, WavPack)
   • All rating values (1–5) round-trip test
   • Clear rating test (export 0, verify no rating returned)
   • No-rating-initially test (fresh file returns nullopt)

Some high level comments before doing a more deep dived review:

* Please make sure to use smart pointer

* Use `snake_case` for new config keys

* Please explain why we need a custom worker as opposed to use the same metadata runner

* We will need to have some test to ensure the supported file types are indeed well exported

Cheers for the notes Antoine. PR should be fixed now. Cheers!

@acolombier Antoine any chance someone could have a peek at this?

@acolombier @JoergAtGithub it would be great if someone could have a look at this. It resolves a longstanding community request to add basic functionality which is sorely lacking in the core product. I've built this (with heavy AI assistance obviously) over a few weeks of evenings, done all the due diligence I could think of, and answered requests immediately.

It would be great if this work could be made available to others, including myself in future builds, and would allow me to recapture 5gb of space on my home drive as well.#

Thanks all.

Hi, we are still assessing our AI policy and aren't yet sure where we stand. As you may be aware, Flathub has recently announced a global ban on AI assisted application code, and not complying would mean having to remove our app from their store (200k installs)

Overall, the fix looks like it is aiming in the right direction, though there is a few unhandled values around numeric conversion, which could use a second pass. I appreciate the code was heavily written with AI, so a careful review of the code before we review it would be appreciated.

This required a full beta cycle. That's why I have changed the base branch to main.

@acolombier The careful review pass you asked for is done — two new commits. It confirmed your suspicion about numeric conversion and turned up a couple of real bugs beyond it.

Numeric conversion (your original point)

• The four copy-pasted conversion implementations are consolidated into src/track/taglib/fmpsrating.h/.cpp, built on TrackRecord::isValidRating()/kMinRating/kMaxRating instead of magic numbers.
• FMPS values are now validated with a regex at the string level before conversion. QString::toDouble() accepts "nan" and "inf", and because Mixxx compiles with -ffast-math, std::isfinite() and NaN range comparisons are unreliable. The previous code genuinely accepted "nan" as a rating under this build configuration.
• POPM conversion now guards the full int domain of TagLib's rating() accessor.

Bugs found in the self-review pass

• Exporting a rating into an MPEG file called save(ID3v2), which defaults to StripOthers in TagLib 2 and silently deleted existing ID3v1/APE tags (e.g. mp3gain's). It now mirrors the arguments of MpegTagSaver::saveModifiedTags().
• A cleared rating was never exported (the removal branch was unreachable), so the stale file tag re-imported the old rating on the next scan. Clearing now removes the FMPS tag and zeroes an existing POPM rating byte, preserving its play counter.
• Export now keeps every tag location the importer consults in sync (APE item on MPEG, ID3v2 frame on FLAC), so a stale secondary location can't resurrect a changed or cleared rating.
• The rating is only written when it differs from the file's value, and the synchronisation timestamp is refreshed afterwards; unchanged saves no longer rewrite files, and rated files no longer appear externally modified.
• Enabling only the rating-export preference no longer triggers a full metadata export of dirty tracks; a rating-only path writes just the rating tag.

One deliberate behaviour change to flag for review: rating import is now fill-only during partial imports (file unchanged since last sync; an existing Mixxx rating wins) and file-wins during full imports (file changed, or first import). Previously the file value unconditionally overwrote the Mixxx rating on every track load, which could silently revert a user's rating. Both preference tooltips have been rewritten to describe the actual behaviour.

Test coverage went from 4 to 27 rating-related tests: format round-trips, malformed and non-finite values, POPM band boundaries and byte preservation, ID3v1/APE preservation on MPEG export, clear/skip semantics at the Track level, and partial-vs-full import semantics. The full suite is green apart from failures that reproduce identically on the merge base without this branch.

On the AI policy: understood that it's still being worked out, no pressure from my side on that. This pass was the careful review you asked for before the deep dive — happy to walk through any part of it.